Skip to content

Conversation

@Zeroto521
Copy link
Contributor

Close to #1074. This is a breaking change. We can release the 5.7.0 version first.

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR merges the Expr and GenExpr classes into a unified expression system. The refactoring replaces the previous dual-system (polynomial Expr and general GenExpr) with a single hierarchy based on Expr, using specialized subclasses like PolynomialExpr, SumExpr, ProdExpr, and various function expressions (ExpExpr, LogExpr, etc.).

Key changes:

  • Unified expression representation with children replacing terms
  • Variable class no longer inherits from Expr
  • Simplified expression tree structure with improved type system
  • Refactored constraint creation methods to use the new expression system

Reviewed Changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
src/pyscipopt/scip.pyi Updated Variable class to remove inheritance from Expr
src/pyscipopt/scip.pxi Modified Variable class structure and added operator overloading methods to delegate to expression system; refactored constraint creation methods to use children instead of terms
src/pyscipopt/scip.pxd Updated Variable class declaration to remove Expr inheritance
src/pyscipopt/propagator.pxi Simplified variable creation by removing unnecessary temporary variable
src/pyscipopt/expr.pxi Complete rewrite of expression system replacing Expr/GenExpr duality with unified class hierarchy

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@Joao-Dionisio Joao-Dionisio marked this pull request as draft November 18, 2025 13:52
@Zeroto521 Zeroto521 marked this pull request as ready for review November 26, 2025 13:26
@Zeroto521
Copy link
Contributor Author

Zeroto521 commented Nov 26, 2025

Finish the base feature and function. And something needs to be done:

  • add test cases
  • add documentation
  • adapt .pyi file
  • cythonize methods
  • update changelog

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 14 out of 14 changed files in this pull request and generated 9 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

raise TypeError("expr must be an Expr instance")
if lhs is None and rhs is None:
raise ValueError(
"Ranged ExprCons (with both lhs and rhs) doesn't supported"
Copy link

Copilot AI Nov 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The error message is grammatically incorrect. It should be "Ranged ExprCons (with both lhs and rhs) isn't supported" or "Ranged ExprCons (with both lhs and rhs) is not supported" instead of "doesn't supported".

Suggested change
"Ranged ExprCons (with both lhs and rhs) doesn't supported"
"Ranged ExprCons (with both lhs and rhs) is not supported"

Copilot uses AI. Check for mistakes.


class ExprCons:
"""Constraints with a polynomial expressions and lower/upper bounds."""
Copy link

Copilot AI Nov 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The docstring says "Constraints with a polynomial expressions" but should say "Constraints with polynomial expressions" (without "a" before "polynomial"). Also note that ExprCons can now handle non-polynomial expressions too (like exp, log, etc.), so the docstring is inaccurate and should be updated to reflect that it handles general expressions.

Suggested change
"""Constraints with a polynomial expressions and lower/upper bounds."""
"""Constraints with general expressions and lower/upper bounds."""

Copilot uses AI. Check for mistakes.
@Joao-Dionisio
Copy link
Member

Thanks for your work @Zeroto521 !! This is something that should be very very well tested before merging. We're also in the middle of a SCIP meeting, so I can bring this up with the guys and we can quickly take a high level look.

@Joao-Dionisio Joao-Dionisio marked this pull request as draft December 8, 2025 19:22
Updated test cases in test_term.py to create explicit Model instances before adding variables. Added assertion for string representation in test_mul for improved test coverage.
Introduced a pytest fixture to create and share a Model, variable, and Term instance across tests. This reduces code duplication and improves test maintainability.
Replaces dictionary comprehension with dict.fromkeys for initializing the parent class in ProdExpr. This change simplifies the code and maintains the same functionality.
Simplifies the type conversion logic in UnaryExpr.to_subclass by removing the explicit conversion of Number to ConstExpr and adjusting the order of type checks.
Introduces a __bool__ method to the Expr class to allow direct truthiness checks. Refactors existing checks for self.children to use the new __bool__ method, improving code readability and consistency.
Refines addition and multiplication logic in Expr, PolynomialExpr, and ProdExpr to better handle cases involving constants 0 and 1. This improves efficiency and correctness by avoiding unnecessary object creation and ensuring proper simplification of expressions.
Replaces usage of the 'terms' variable with 'children' to improve clarity and consistency when accessing constraint expression children in Model methods. No functional changes were made.
Changed the parameter name from 'idx' to 'key' in the Term.__getitem__ method for consistency and clarity.
Implemented the __iter__ method for the Term class to allow iteration over its variables. Updated internal usage to leverage the new iterator.
Changed the _hash attribute in the Term class to be readonly and updated the __eq__ method to check for instance type before comparing hashes. This improves type safety and encapsulation.
Replace TypeError checks with inequality assertions when comparing Term instances to non-Term objects. This reflects updated behavior where such comparisons return False instead of raising an exception.
Refined the __hash__ implementations to include type information and use _children instead of children. This ensures more robust and type-aware hashing for expression objects.
Explicitly cast values to float and Expr types in ExprCons methods to ensure type correctness and avoid potential type errors. This affects normalization and comparison operator overloads.
Replaces the UnaryOperatorMixin with a new ExprLike base class to unify the interface for expressions and variables. Refactors Expr, Variable, and related classes to use this new structure, moving operator overloads and common logic to ExprLike. Direct instantiation of Expr and Variable is now disallowed, and Variable now maintains an internal PolynomialExpr view for expression operations. Updates type checks and internal helpers to support the new design, improving maintainability and extensibility.
Replaces _is_const checks with explicit type checks for ConstExpr throughout expression operations for clarity and correctness. Introduces a _reset_hash helper to standardize hash invalidation, replacing direct assignments to _hash. Simplifies and corrects logic in several arithmetic and comparison methods, and updates __repr__ for UnaryExpr to improve output consistency.
Changed setObjective and chgReoptObjective to accept ExprLike instead of Expr, updated parameter types and default values for clarity, and removed an unnecessary assertion in setObjective. These changes improve type flexibility and documentation accuracy.
Simplifies the __mul__ method in PolynomialExpr by removing redundant checks and streamlining the multiplication logic. Also updates casting from Cython-style to Python-style in several places for consistency.
Refactored the matrix class hierarchy by renaming MatrixBase to MatrixExprLike in both matrix.pxi and scip.pxi. Updated all relevant class inheritances to improve code clarity and consistency.
Moved the conversion of 'other' to an Expr earlier in the __mul__ method to improve code clarity and ensure type checks use the converted value.
Updated the __hash__ and ptr methods in the Variable class to return size_t instead of Py_hash_t for consistency and type correctness.
Removed __array_priority__ from ExprLike and set MatrixExprLike's __array_priority__ to 100. This change ensures consistent behavior when interacting with NumPy ufuncs and resolves potential priority conflicts.
Replaces repeated (Number, Variable, Expr) type checks with centralized EXPR_OP_TYPES and NUMBER_TYPES tuples. This improves maintainability and consistency in type checking throughout the expression classes.
Type hints were removed from the __hash__ and ptr methods in the Variable class to improve compatibility and simplify the code.
Refactored Expr and related classes to use the public attribute 'children' instead of the internal '_children'. Updated all references and property definitions accordingly for consistency and clarity.
Updated references from cons.expr._children to cons.expr.children in the Model class to use the correct attribute for accessing constraint expression children. This change improves code clarity and ensures compatibility with the current expression API.
Improves detection and conversion of single-term polynomials by introducing _is_single_poly and _to_poly helpers. Removes redundant __add__ override in PolynomialExpr and streamlines sum logic in Expr to ensure correct type conversion for single polynomials. Updates __repr__ in UnaryExpr for consistency with new helpers.
Changed the first parameter of _expr_cmp from 'self' to 'expr' to improve clarity and consistency, updating all internal references accordingly.
Moved the _to_dict function from the Expr class to a standalone cdef function for improved clarity and reusability. Updated all internal calls to use the new function signature and removed the method declaration from the class definition.
Refines handling of exponentiation for Expr and PolynomialExpr classes, including special cases for exponents 0 and 1, and optimizes polynomial multiplication by using a dictionary for intermediate results. These changes improve correctness and efficiency in mathematical operations.
Introduces a new _normalize function to handle normalization and coefficient scaling of expression children. Replaces multiple dictionary comprehensions with calls to _normalize for improved code reuse and clarity.
Refactored the _to_dict function to use C-level dict access for improved efficiency and to handle zero coefficients correctly. This change avoids unnecessary copying and ensures more accurate updates when combining expression children.
Refactors the __mul__ method in PolynomialExpr to use PyDict_Next for iterating over dictionary items, improving performance and reducing Python overhead. This change also skips zero coefficients and accumulates products directly in the result dictionary.
Replaces the use of next(iter(expr.children)) with PyDict_Next for more direct and efficient access to the first child key in Expr. Raises StopIteration if Expr has no children.
Refactored the __mul__ method in the Term class to efficiently merge variable tuples while maintaining order based on hash values. Introduced a new _extend helper function for extending lists from tuple slices, improving performance and code clarity.
Replaces direct tuple item comparison with explicit Variable extraction and identity check in the Term class equality method. This ensures correct behavior when comparing Term objects.
Removed unnecessary checks for coefficient equal to 1 in multiplication logic and updated normalization to directly copy children when coefficient is 1. This streamlines expression handling and avoids redundant copying.
Deleted the unused internal _extend function to clean up the codebase and improve maintainability.
Deleted custom implementations of exp, log, sqrt, sin, and cos functions, as well as related helper code. These functions are now directly assigned from numpy, simplifying the code and reducing redundancy.
Prevents direct instantiation of ExprCons by raising NotImplementedError in __init__, and introduces a private _expr_cons factory function for internal creation. Updates all internal usages to use _expr_cons, improving encapsulation and guiding users to use comparison operators for constraint creation.
Refactors __pow__ and __rpow__ methods to handle edge cases such as zero and negative exponents or bases more robustly. Adds explicit error handling for invalid operations and clarifies type checks for constant expressions.
Replaces explicit checks for unary numpy ufuncs with a mapping (UNARY_MAP) from ufuncs to method names, simplifying and centralizing the dispatch logic for unary operations in ExprLike.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants